Add sharding support #1164
Add sharding support #1164
Conversation
WalkthroughAdded a new optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant Client
participant Server
rect rgb(235,245,255)
Test->>Client: add_or_update_networks(body with sharding: true, remotes with writeApiKey)
Client->>Server: HTTP POST /networks { sharding: true, self:..., remotes: [...] }
Server-->>Client: 200 OK (networks accepted)
Client-->>Test: return response (includes sharding: true)
end
rect rgb(255,245,235)
Test->>Client: get_all_networks()
Client->>Server: HTTP GET /networks
Server-->>Client: 200 OK (networks payload)
Client-->>Test: return networks (assert match)
end
rect rgb(245,255,235)
Test->>Client: disable_sharding(client)
Client->>Server: HTTP POST /networks { sharding: false }
Server-->>Client: 200 OK
Client-->>Test: teardown complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/client/test_client_multi_search_meilisearch.py (1)
107-107: Add assertion to verify sharding was configured.The response from
add_or_update_networksshould be validated to confirm that sharding was successfully enabled, similar to the assertion intest_client_network.pyline 37.Apply this diff to add the missing assertion:
assert "results" not in resp + assert resp["sharding"] is True assert "results" not in response
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
meilisearch/models/task.py(1 hunks)tests/client/test_client_multi_search_meilisearch.py(1 hunks)tests/client/test_client_network.py(1 hunks)tests/client/test_client_sharding.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/client/test_client_network.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(779-799)
tests/client/test_client_sharding.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (2)
add_or_update_networks(779-799)get_all_networks(801-814)
🔇 Additional comments (4)
tests/client/test_client_multi_search_meilisearch.py (1)
84-101: LGTM! Network configuration correctly includes sharding and writeApiKey fields.The addition of
"sharding": Trueand"writeApiKey"fields to the network configuration aligns with the PR's objective of introducing sharding support.tests/client/test_client_network.py (1)
17-37: LGTM! Test properly validates sharding configuration.The test correctly adds the
shardingflag andwriteApiKeyfields to the network configuration, and includes appropriate assertions to verify the response.tests/client/test_client_sharding.py (1)
1-35: LGTM! Comprehensive test coverage for sharding functionality.This new test file provides thorough validation of the sharding feature, including:
- Setting network configuration with sharding enabled
- Validating writeApiKey fields are properly stored and retrieved
- Confirming the sharding flag persists across update/get operations
meilisearch/models/task.py (1)
24-24: The Task.network field is correctly implemented and will deserialize from API responses.The field definition follows the correct pattern for optional generic dictionaries. When the network experimental feature is enabled, the field will contain replication metadata (origin or remotes data), and Pydantic will automatically deserialize it from the API response. The existing network tests in the codebase validate network configuration endpoints; the field itself will be populated transparently when tasks are retrieved.
jawad-khan
force-pushed
the
jawad/sharding-support
branch
from
d43103a to
48b1dd0
Compare
|
@jawad-khan thanks for your PR |
curquiza
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_utils.py (1)
36-37: Helper implementation looks correct; consider minor clarity improvementsThe helper cleanly delegates to
client.add_or_update_networkswith a minimal payload and is appropriate for test reuse. To make its intent clearer to future readers, you might optionally add a short docstring and a return type hint (-> None), since this will likely be shared across multiple test modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
meilisearch/models/task.py(1 hunks)tests/client/test_client_multi_search_meilisearch.py(3 hunks)tests/client/test_client_network.py(2 hunks)tests/client/test_client_sharding.py(1 hunks)tests/test_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- meilisearch/models/task.py
- tests/client/test_client_multi_search_meilisearch.py
- tests/client/test_client_network.py
- tests/client/test_client_sharding.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_utils.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(779-799)
jawad-khan
force-pushed
the
jawad/sharding-support
branch
from
c25fc71 to
bea059e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_utils.py (1)
36-37: Add type hints and docstring to the helper function.The
disable_shardingfunction would benefit from type hints and a docstring to improve maintainability and clarify its purpose as a test cleanup utility.Apply this diff to add type hints and documentation:
-def disable_sharding(client): +def disable_sharding(client) -> None: + """Disable sharding on the Meilisearch client. + + This is a test helper function to clean up sharding state after tests. + + Parameters + ---------- + client + Meilisearch client instance + """ client.add_or_update_networks(body={"sharding": False})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
meilisearch/models/task.py(1 hunks)tests/client/test_client_multi_search_meilisearch.py(3 hunks)tests/client/test_client_network.py(2 hunks)tests/client/test_client_sharding.py(1 hunks)tests/test_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/client/test_client_multi_search_meilisearch.py
- tests/client/test_client_network.py
- tests/client/test_client_sharding.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_utils.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(893-913)
🔇 Additional comments (1)
meilisearch/models/task.py (1)
24-24: LGTM!The addition of the
networkfield to the Task model is appropriate and follows the existing patterns. The typeOptional[Dict[str, Any]]is consistent with other flexible dictionary fields likedetailsanderror, and no validation is needed for this optional metadata field.
|
@Strift Test cases aren't failing anymore, PR is final from my side. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_utils.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(893-913)
🪛 GitHub Actions: Tests
tests/test_utils.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by 'black'. Run 'pipenv run black' to fix formatting issues.
|
|
||
| # Refactor to use the unified API to toggle experimental features | ||
| def disable_sharding(client): | ||
| client.add_or_update_networks(body={"sharding": False}) |
There was a problem hiding this comment.
Fix Black formatting to unblock the pipeline.
The pipeline failed because this file doesn't conform to Black's formatting standards.
Run the following command to fix the formatting:
pipenv run black tests/test_utils.py🤖 Prompt for AI Agents
In tests/test_utils.py around lines 34 to 37 the file fails Black formatting;
reformat the file to conform to Black standards by running the formatter (for
example: pipenv run black tests/test_utils.py) or apply Black’s formatting rules
manually (ensure proper spacing, line lengths, and blank lines) and commit the
reformatted file so the pipeline passes.
| def disable_sharding(client): | ||
| client.add_or_update_networks(body={"sharding": False}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints for clarity.
The function signature lacks type hints for the client parameter and return type, which would improve code clarity and enable better IDE support.
Apply this diff to add type hints:
-def disable_sharding(client):
+def disable_sharding(client: "meilisearch.Client") -> None:
client.add_or_update_networks(body={"sharding": False})Optionally, add a docstring to document the helper's purpose:
def disable_sharding(client: "meilisearch.Client") -> None:
"""Disable sharding experimental feature via the network API."""
client.add_or_update_networks(body={"sharding": False})🤖 Prompt for AI Agents
In tests/test_utils.py around lines 36 to 37, add type hints to the
disable_sharding function signature and return type (e.g., client:
"meilisearch.Client" -> None) to improve clarity and IDE support; update the
function definition accordingly and optionally add a one-line docstring
describing its purpose ("Disable sharding experimental feature via the network
API.") — use a string forward reference if meilisearch.Client is not imported at
top.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_utils.py (1)
37-38: The function works correctly.The helper correctly disables sharding by calling the network API. The implementation is simple and appropriate for a test utility.
Consider adding type hints for better IDE support and code clarity:
-def disable_sharding(client): +def disable_sharding(client: "meilisearch.Client") -> None: + """Disable sharding experimental feature via the network API.""" client.add_or_update_networks(body={"sharding": False})Note: The Black formatting issue mentioned in past reviews appears to be resolved with the proper blank lines now in place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_utils.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(893-913)
| iso_to_date_time("2023-07-13T23:37:20Z") | ||
|
|
||
|
|
||
| # Refactor to use the unified API to toggle experimental features |
There was a problem hiding this comment.
Clarify or remove the comment.
The comment suggests a future refactor ("Refactor to use the unified API..."), but the code already uses add_or_update_networks, which appears to be the unified API. Either remove the comment if the refactor is complete, or reword it to accurately describe the function's current purpose.
Consider replacing with a descriptive comment:
-# Refactor to use the unified API to toggle experimental features
+# Helper to disable sharding via the network API
def disable_sharding(client):
client.add_or_update_networks(body={"sharding": False})Or remove the comment entirely if the function name is self-explanatory.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Refactor to use the unified API to toggle experimental features | |
| # Helper to disable sharding via the network API | |
| def disable_sharding(client): | |
| client.add_or_update_networks(body={"sharding": False}) |
🤖 Prompt for AI Agents
In tests/test_utils.py around line 36, the inline comment "Refactor to use the
unified API to toggle experimental features" is outdated or misleading because
the code already calls add_or_update_networks; update the comment to accurately
describe what the test helper does (e.g., "Use add_or_update_networks to
configure test networks") or remove the comment entirely if the function name is
self-explanatory, ensuring the remaining comment (if any) is concise and
describes current behavior.
3 participants
Pull Request
This PR introduces sharding support to the Meilisearch Python SDK. I have tested the sharding functionality locally using two remote nodes, and all related test cases passed successfully.
Related issue
Fixes #<1137>
What does this PR do?
As mentioned in the issue, I have tested all of these points locally and added tests for the parts that can be covered within the test suite.
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.